Email the lease holder when an operator revokes their access by maxkpower · Pull Request #8292 · bitwarden/server

maxkpower · 2026-08-31T14:14:12Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-42817

📔 Objective

Emails a lease holder when an operator revokes their active access. The push already re-locks their client; this tells them why.

Sent only when AccessLeaseAction.Revoked. A holder ending their own lease (Cancelled) receives nothing: mailing someone about their own action is what trains people to ignore the channel.
Says "revoked", matching pamStatusRevoked and pamAuditKindLeaseRevoked in the web vault. The product reserves "ended" for the holder's own action.
States that the access cannot be resumed and that regaining it needs a new request.
The revocation reason is not included and stays on the request.

This is a courtesy, not a control: the lease is already dead server side before the mail is composed.

Top of a four PR stack on pam/uat. Sits on the decision notification PR.

📸 Screenshots

github-actions · 2026-08-31T14:15:08Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new LeaseRevokedMailNotifier with its interface and TryAddScoped registration, the RevokeAccessLeaseCommand call site, the AccessLeaseRevokedView/AccessLeaseRevokedMail pair, the MJML source against the compiled .html.hbs and the .text.hbs, and the three test suites. The notifier mirrors RequesterMailNotifier and ApproverMailNotifier — FeatureFlagKeys.Pam pre-guard ahead of the organization read, warn-and-return on an unresolved organization, and a catch-and-log boundary so a mail outage cannot fail a revocation already written — and the call site awaits it unguarded exactly as DecideAccessRequestCommand and SubmitAccessRequestCommand await theirs. The AccessLeaseAction.Revoked gate correctly excludes Cancelled, the only other value endAction can take, and both directions are pinned in the command tests; ScheduledEnd reads lease.NotAfter, which neither the Dapper AccessLease_Revoke procedure nor the EF ExecuteUpdateAsync path mutates, so "due to run until" reports the original window, and IsLive(now) keeps that instant in the future as the doc comment claims. Zero-knowledge boundaries hold: only the organization name and scheduled end reach the body, the reason and cipher stay on the request (pinned by reflection over the view's properties), OrganizationName renders escaped, and both log statements carry ids only.

Code Review Details

No blocking findings.

Also checked: RevokeAccessLeaseCommand is the only path that stamps AccessLeaseAction.Revoked — CancelAccessRequestCommand refuses any request that has minted a lease and redirects to revoke — so there is no second operator-ends-access path left unmailed. Core.csproj embeds **\*.hbs, and the rendering spec resolves the template by type name, which is what catches a misnamed or misplaced file.

The one failing check, Aikido on {{{Url}}} at AccessLeaseRevokedView.html.hbs:211, is the triple-brace interpolating only GlobalSettings.BaseServiceUri.VaultWithHash and a Guid through PamAccessMailView.Url; both sibling templates do the same, and the author's reply on that thread is correct.

PR Metadata Assessment

❓ QUESTION: The Screenshots section is empty for a new customer-facing email — a rendered preview of AccessLeaseRevokedView would let reviewers check copy and layout before it ships.

codecov · 2026-08-31T16:59:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (pam/PM-42817/mail-request-decided@caca1cd). Learn more about missing BASE report.

Additional details and impacted files

@@                         Coverage Diff                          @@
##             pam/PM-42817/mail-request-decided    #8292   +/-   ##
====================================================================
  Coverage                                     ?   64.80%           
====================================================================
  Files                                        ?     2607           
  Lines                                        ?   112103           
  Branches                                     ?    10040           
====================================================================
  Hits                                         ?    72648           
  Misses                                       ?    37035           
  Partials                                     ?     2420

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

aikido-pr-checks · 2026-09-09T14:11:34Z

+        <tbody>
+          <tr>
+            <td align="center" bgcolor="#175ddc" role="presentation" style="border:none;border-radius:20px;cursor:auto;mso-padding-alt:12px 24px;background:#175ddc;" valign="middle">
+              <a href="{{{Url}}}" style="display:inline-block;background:#175ddc;color:#ffffff;font-family:'Helvetica Neue', Helvetica, Arial, sans-serif;font-size:16px;font-weight:600;line-height:120%;margin:0;text-decoration:none;text-transform:none;padding:12px 24px;mso-padding-alt:0px;border-radius:20px;" target="_blank">


Rendering unescaped input in handlebar/mustache template can lead to XSS attacks - high severity
Using {{{ }}} renders an unescaped string into the template. This means the developer is responsible for allowlisting all HTML strings that can be passed in.

Show fix

Remediation: Avoid {{{ }}} usage. Ignore this issue only if the HTML content you feed into it is static. Defend in depth against XSS attacks by using a strict CSP policy. If you must use HTML, sanitize it with a library such as DOMPurify.

_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

server-composed URL, no user input

…d-controls

Also trims the mail notification doc comments.

patriksvensson · 2026-09-11T12:38:59Z

+        _accessMailNotifier = accessMailNotifier;
+        _organizationRepository = organizationRepository;
+        _globalSettings = globalSettings;
+        _featureService = featureService;
+        _logger = logger;


Needs null checks, such as _foo = foo ?? throw new ArgumentNullException(nameof(foo)).

…il-lease-revoked

maxkpower added the ai-review Request a Claude code review label Aug 31, 2026

maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from e1f25bc to f81d164 Compare August 31, 2026 16:33

maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from f81d164 to 917d83d Compare August 31, 2026 20:26

maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from 917d83d to 18ea011 Compare September 9, 2026 14:11

aikido-pr-checks Bot reviewed Sep 9, 2026

View reviewed changes

maxkpower added the t:feature Change Type - Feature Development label Sep 10, 2026

maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from 18ea011 to 962267b Compare September 10, 2026 18:01

maxkpower added 7 commits September 10, 2026 22:56

Notify the lease holder by email when an operator revokes their access

7f765de

Simplify the lease-revocation notification

304eed3

Drop narrating comments from the mail-lease-revoked changes

452a467

Point the revocation mail link at the pam route rather than privilege…

94293ec

…d-controls

Gate the revocation mail on the existing PAM flag rather than its own

2a0a450

Say revoked rather than ended in the revocation email

469b5b4

Inherit the shared mail view in the revocation email

a231580

Also trims the mail notification doc comments.

maxkpower force-pushed the pam/PM-42817/mail-lease-revoked branch from 962267b to a231580 Compare September 10, 2026 20:56

patriksvensson approved these changes Sep 11, 2026

View reviewed changes

Merge branch 'pam/PM-42817/mail-request-decided' into pam/PM-42817/ma…

b09ef78

…il-lease-revoked

Navigation Menu

Uh oh!

Email the lease holder when an operator revokes their access - #8292

Email the lease holder when an operator revokes their access#8292
maxkpower wants to merge 8 commits into
pam/PM-42817/mail-request-decidedfrom
pam/PM-42817/mail-lease-revoked

maxkpower commented Aug 31, 2026

Uh oh!

github-actions Bot commented Aug 31, 2026 •

edited

Loading

PR Metadata Assessment

Uh oh!

codecov Bot commented Aug 31, 2026 •

edited

Loading

Uh oh!

aikido-pr-checks Bot Sep 9, 2026

Uh oh!

maxkpower Sep 10, 2026

Uh oh!

patriksvensson Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

maxkpower commented Aug 31, 2026

🎟️ Tracking

📔 Objective

📸 Screenshots

Uh oh!

github-actions Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

PR Metadata Assessment

Uh oh!

codecov Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aikido-pr-checks Bot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

maxkpower Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

patriksvensson Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 31, 2026 •

edited

Loading

codecov Bot commented Aug 31, 2026 •

edited

Loading